Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use PyProj instead of Cartopy for internal coordinate transforms and rename crs coordinate #1483

Merged
merged 9 commits into from
Oct 9, 2020
Merged

Use PyProj instead of Cartopy for internal coordinate transforms and rename crs coordinate #1483

merged 9 commits into from
Oct 9, 2020

Conversation

jthielen
Copy link
Collaborator

@jthielen jthielen commented Aug 27, 2020 •
edited by dopplershift
Loading

Description Of Changes

Implements the changes discussed in #1455 (comment). Part of this was changing the API for lat_lon_grid_deltas to take the PyProj Geod directly rather than by constructor kwargs (makes it easier to handle from grid_deltas_from_dataarray, which extracts the Geod from the PyProj CRS).

Marking as draft/work-in-progress for now since the following have yet to be done:

  • Update azimuth_range_to_lat_lon and any other Geod uses to take geod arg rather than kwargs to construct new Geod
  • Make pyproj >= 2.2 a required dependency (unmodified as of now)
  • Check if pyproj 3.0.0 breaks any APIs that would necessitate a forward-looking pin
  • Diagnose the failure of 4D kinematics tests (want to know what changed exactly with grid delta calculations, and see if we were in error before and this fixes it, or if something broke with update)
  • Add tests of new pyproj helper methods

If someone wanted to take these items on to speed up the process, that would be great! Otherwise, I'll loop back to this after the xarray stuff for 1.0.

Checklist

@jthielen jthielen added Area: Calc Pertains to calculations Area: Cross-sections Pertains to making cross-sections through data Area: Projections Pertains to projecting coordinates between coordinate systems Area: Xarray Pertains to xarray integration Type: API Change Changes to how existing functionality works Type: Maintenance Updates and clean ups (but not wrong) labels Aug 27, 2020
@jthielen jthielen mentioned this pull request Sep 3, 2020
Merged
6 tasks
@jthielen jthielen added this to the 1.0 milestone Sep 15, 2020
@jthielen jthielen force-pushed the cartopy-to-pyproj-calc branch from 89a5378 to 85cffcd Compare October 1, 2020 21:16
@jthielen
Copy link
Collaborator Author

jthielen commented Oct 1, 2020

Just fixed up the rebase of this on latest master after the changes of #1490, but the todo list above remains.

@dopplershift dopplershift force-pushed the cartopy-to-pyproj-calc branch from 85cffcd to 011f45a Compare October 2, 2020 20:12
@dopplershift
Copy link
Member

I looked at the list on Python 3, and API wise I think we'll be fine. However, they are dropping older versions of PROJ, and doing enough that I think the right approach is to make our PyProj dependency right now >=2.2,<3.0. We have the testing infrastructure that will make it easy to evaluate how 3 works for us, but I don't think there's any need to rush into support for PyProj 3.0.

@dopplershift dopplershift force-pushed the cartopy-to-pyproj-calc branch 2 times, most recently from 46873e6 to be31c7d Compare October 6, 2020 05:09
@dopplershift
Copy link
Member

The reason for the test failures is that before for...CartoPy reasons...we were using PlateCarree for lat/lon projections--which doesn't support globes. Thus, when converting that to a PyProj Geod, we ended up using the default (WGS84). Now by using PyProj directly, we create a correct Geod instance from the metadata (for the tests this is actually a sphere with a particular radius). For our test dataset, the difference is ~1km on a ~350km spacing.

Before updating all the test values, I was able to make the tests pass by hacking the test metadata to specify a WGS84 spheroid, so that's clearly the issue.

Further evidence that shifting over to using PyProj directly for calcs is a good thing to do.

@dopplershift dopplershift force-pushed the cartopy-to-pyproj-calc branch from be31c7d to 3fdb776 Compare October 6, 2020 08:23
@dopplershift
Copy link
Member

Bumping our minimum pyproj to 2.3 was enough to make tests pass for me locally. Assuming that holds on CI, let's just go with that (released Aug 2019) rather than trying to do any special work-arounds.

@dopplershift dopplershift marked this pull request as ready for review October 6, 2020 08:34
@dopplershift dopplershift force-pushed the cartopy-to-pyproj-calc branch from 3fdb776 to 588784c Compare October 6, 2020 08:44
dcamron
dcamron previously approved these changes Oct 6, 2020
View reviewed changes
Copy link
Member

@dcamron dcamron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like these changes, and didn't find any other issues myself!

.travis.yml Show resolved Hide resolved
src/metpy/xarray.py Show resolved Hide resolved
@dopplershift dopplershift force-pushed the cartopy-to-pyproj-calc branch from 8272a02 to cab7bd9 Compare October 7, 2020 05:36
@dopplershift
MNT: Update tests due to change to PyProj
ed2cd6c 
Necessary because we are now properly using the Earth ellipsoid that is
present in the data for lat/lon projection. Previously, we were using
CartoPy's PlateCarree, which doesn't support globes--thus when we got
the spheroid we used the CartoPy default, which is WGS84.

Was able to reproduce previous test values by overriding data ellipsoid
to specify WGS84. Updating the test values is better long-term than
putting a WGS84 hack into the data fixture.
@dopplershift
MNT: Don't use deprecated functions in GitHub Actions
3686ea7 
Apparently these have been changed for security reasons:
https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/
@dopplershift dopplershift force-pushed the cartopy-to-pyproj-calc branch from cab7bd9 to 038872e Compare October 7, 2020 05:43
@dopplershift dopplershift force-pushed the cartopy-to-pyproj-calc branch from 038872e to ee2117d Compare October 7, 2020 05:54
@dopplershift dopplershift changed the title WIP: Use PyProj instead of Cartopy for internal coordinate transforms and rename crs coordinate Use PyProj instead of Cartopy for internal coordinate transforms and rename crs coordinate Oct 9, 2020
@dopplershift dopplershift merged commit f8d2fcd into Unidata:master Oct 9, 2020
@jthielen jthielen deleted the cartopy-to-pyproj-calc branch January 18, 2021 02:27
@leifdenby leifdenby mentioned this pull request Nov 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcamron dcamron dcamron approved these changes

@dopplershift dopplershift dopplershift approved these changes

Assignees
No one assigned
Labels
Area: Calc Pertains to calculations Area: Cross-sections Pertains to making cross-sections through data Area: Projections Pertains to projecting coordinates between coordinate systems Area: Xarray Pertains to xarray integration Type: API Change Changes to how existing functionality works Type: Maintenance Updates and clean ups (but not wrong)
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.

Use PyProj instead of CartoPy for calculations
3 participants
@jthielen @dopplershift @dcamron